-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: build pg-cloudflare
as a CommonJS module
#3168
base: master
Are you sure you want to change the base?
Conversation
pg-cloudflare
to a CommonJS modulepg-cloudflare
as a CommonJS module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh dear. CI doesn't agree...
https://github.com/brianc/node-postgres/actions/runs/8256843684/job/22602824382?pr=3168#step:8:1100
The problem with converting this module to CommonJS is this line:
If we compile this code to CommonJS this becomes a dynamic require, which the Wrangler bundler then complains about, since the esbuild output results in a dynamic `require("cloudflare:sockets") call. Could we solve this instead by marking the package as an ESM module (i.e. |
Not in this case. These files don't get transformed by Vite, and are imported using the |
1951ca2
to
8d97107
Compare
Looks like the magic incantation is |
Hey! 👋 We're currently working on an integration between Cloudflare Workers and Vitest, allowing you to run your Vitest tests inside the Workers runtime. A community member tried out an early version of this integration and uncovered an issue with
pg
/pg-cloudflare
(cloudflare/workers-sdk#5127 (comment)).In
pg/lib/stream.js
,pg-cloudflare
isrequire()
ed...node-postgres/packages/pg/lib/stream.js
Line 10 in b4bfd63
...but
pg-cloudflare
is compiled to an ES module. ES modules can't berequire()
d so this shouldn't work. Fortunately, Workers are usually bundled withesbuild
which papers over differences between the module formats and allowed this to work. Our Vitest integration uses a "bundle-less" approach that imports modules at runtime from disk more like Node. This meantpg-cloudflare
couldn't berequire()
d bypg
, failing any Workers tests usingpg
.This PR updates
pg-cloudflare
'stsconfig.json
to compile to CommonJS instead, fixing this issue. 👍/cc @petebacondarwin